Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OLM disconnected config #16458

Merged
merged 1 commit into from
Sep 24, 2019
Merged

Conversation

@adellape adellape added this to the Future Release milestone Aug 30, 2019
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2019
@adellape
Copy link
Contributor Author

adellape commented Sep 4, 2019

@ecordell @shawn-hurley FYI on this WIP.

@adellape adellape force-pushed the olm_disconnected branch 2 times, most recently from 18c07f5 to 5998ab8 Compare September 6, 2019 15:24
@adellape adellape changed the title [WIP] Add OLM disconnected config Add OLM disconnected config Sep 6, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2019
@adellape adellape force-pushed the olm_disconnected branch 3 times, most recently from b4ab0bf to a692f1e Compare September 6, 2019 15:57
@adellape adellape added the peer-review-needed Signifies that the peer review team needs to review this PR label Sep 6, 2019
@adellape
Copy link
Contributor Author

adellape commented Sep 6, 2019

@openshift/team-documentation PTAL for peer review.

CMD ["--database", "bundles.db"]
----

.. Use the `podman` command to create and tag the container image from the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want customers building an image?
Shouldn't we be providing this image to customers?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea who would be responsible for building/pushing and managing these images.

community-operators and certified-operators and redhat-operators are expecting changes, often by individual teams. I don't know how we keep this image up to date or how to tie this to an errata/release process.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sosiouxme maybe we can chat about this offline?

└─ <CSVs_and_CRDs_and_a_package_file>
----

... If you see files already in the structure described in the previous step, then you do not have to do anything.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't they be in this format (based on the instructions we are giving in the piro step?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there are two different things in the tar potentially. one that is just bundle.yaml and one that is broken up already. This is stating that if you don't see the bundle.yaml then you don't need to do anything. Can we make that more clear? Does that make sense?

$ curl https://quay.io/cnr/api/v1/packages?namespace=certified-operators >> packages.txt
----
+
Each package in the new `packages.txt` is an Operator that you could add to your
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we explain to customers how to parse this?

Can we provide them a bash script that does all this?

@adellape adellape force-pushed the olm_disconnected branch 4 times, most recently from a083dfe to dbf5d5a Compare September 11, 2019 20:01
@adellape
Copy link
Contributor Author

adellape commented Sep 11, 2019

@jianzhangbjz @bandrade @scolange PTAL for QE review.

Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! A couple nitpicks.

$ curl -XGET https://quay.io/cnr/api/v1/packages/community-operators/etcd/blobs/sha256/8108475ee5e83a0187d6d0a729451ef1ce6d34c44a868a200151c36f3232822b -o etcd.tar.gz
----

.. To pull the information out, you must untar the archive into a directory with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "pull the information out", what about "To extract the information"?
Same with line 70 above: "use it to extract the gzipped archive"

@sheriff-rh sheriff-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 11, 2019
Copy link

@bandrade bandrade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adellape Just added a suggestion but LGTM for me.


.. In this directory, you must now handle two different types of bundles:

... If you see a single file called `bundle.yaml`, you must break the content under

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really concerned about letting the user do this step, but I know that are improvements being implemented. Is it possible to add the bundle.yaml in this doc used for this example? That would help the user to associate with their use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adellape, lgmt.

@adellape adellape force-pushed the olm_disconnected branch 2 times, most recently from 8f40a6d to 23abd99 Compare September 23, 2019 18:03
@adellape adellape force-pushed the olm_disconnected branch 2 times, most recently from 7f5aaa3 to fad25c3 Compare September 23, 2019 20:40
apiVersion: config.openshift.io/v1
kind: OperatorHub
spec:
disableAllDefaultSources: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, you can set this with a copy/pastable command like:

$ oc patch OperatorHub cluster --type json -p '[{"op": "add", "path": "/spec/disableAllDefaultSources", "value": true}]'

to avoid dropping into an editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Updated.

@adellape adellape merged commit 625bc4c into openshift:master Sep 24, 2019
@adellape adellape deleted the olm_disconnected branch September 24, 2019 21:33
@adellape
Copy link
Contributor Author

/cherrypick enterprise-4.2

@openshift-cherrypick-robot

@adellape: new pull request created: #16866

In response to this:

/cherrypick enterprise-4.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

+
----
$ oc image mirror quay.io/coreos/etcd-operator \
<internal_registry_route>:5000/coreos/etcd-operator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameterize this port? e.g. we use <local_registry_host_name>:<local_registry_host_port> in a number of other places in the enterprise-4.2 branch (CC @kalexand-rh).

@@ -511,6 +511,9 @@ Topics:
- Name: Creating policy for Operator installations and upgrades
File: olm-creating-policy
Distros: openshift-origin,openshift-enterprise
- Name: Configuring OLM in disconnected mode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The release mirroring PRs went with "restricted network" instead of "disconnected" (e.g. see #16696).

@adellape
Copy link
Contributor Author

@wking @bandrade Thank you, following up via #16892.

Copy link
Contributor

@sferich888 sferich888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vikram-redhat please re-review this PR and look at the denoted comments and consider them for inclusion.

$ mkdir -p manifests/etcd/ <1>
$ tar -xf etcd.tar.gz -C manifests/etcd/
----
<1> Create different subdirectories for each extracted archive so that files are not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be a call out or warning.

@adellape can you consider modifying this?

Copy link
Contributor Author

@adellape adellape Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A <#> is considered a "callout icon" in AsciiDoc, and I think in this case it's better to keep the connection / context using the numbered icon on the line item (switching to a Warning box would lose that and doesn't seem to add much).

Dockerfile:
+
----
$ podman build -f custom-registry.Dockerfile \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adellape we need to add a note about setting up podman (to authenticate with the customer portal).
Without this, the build will fail because of the registry being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we can't link to modules from within a module, I had to add the link to the registry login procedure in the "Additional resources" at the bottom of the assembly:

http://file.rdu.redhat.com/~adellape/083019/olm_disconnected/applications/operators/olm-disconnected.html

disconnected {product-title} cluster
+
----
$ podman push <internal_registry_route>/<namespace>/custom-registry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we specify what namespace to push this too?
What authentication needs to be provided to podman for this push to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ecordell @shawn-hurley My understanding was it could be any namespace. Should we state one? Existing or have them create a new one?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same registry and info that is in the main disconnected doc correct? we should just follow that IMO.

└── package.yaml
----
+
If you see files already in this structure, you can skip this step. However, if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adellape I think we also need to tell the user to record or pull out the image that is used by the operator versions. We need this for step 8.

Copy link
Contributor Author

@adellape adellape Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as a new step 5 in #16892.

You should also be able to view them from the *OperatorHub* page in the web
console.

. **Mirror the images required by the Operators you want to use.**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In steps 3 and/or 4 you need to tell the user to pull out or record the image path/url. It is needed in this step.

@vikram-redhat
Copy link
Contributor

LGTM from QE received in the followup PR: #16892. Removing QE required flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.2 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants